Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow awaiters to be skipped by setting an annotation #417

Merged
merged 2 commits into from
Feb 12, 2019

Conversation

lblackstone
Copy link
Member

  • Generalize annotation handling
  • Create "pulumi.com/skipAwait" annotation
  • Skip await logic for any resource with the skipAwait annotation set to true

Fixes #179

- Generalize annotation handling
- Create "pulumi.com/skipAwait" annotation
- Skip await logic for any resource with the skipAwait annotation set to true
- Rename annotations package to metadata
- Check against a known list of internal annotations rather than a prefix
@joeduffy
Copy link
Member

Should we also support a global config option for both of these?

This way, you could do

$ pulumi config set kubernetes:skipAwait true

if you want to disable this globally, without needing to edit all of your code.

@lblackstone
Copy link
Member Author

@joeduffy My initial thought was that this should be an "escape hatch" toggle, and should be a little bit inconvenient to set since it bypasses the supported path. Basically, this gives users a way to move forward if they hit a bug or unsupported edge case until we can get it fixed properly.

That said, I'm willing to reconsider if users find this approach to be impractical.

Copy link
Contributor

@metral metral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for the work on this.

Should we also support a global config option for both of these?

I too was torn on the methodology - on one hand making it a config option is almost too easy to disable, while on the other the code annotations is more explicit, but difficult to manage. I see this primarily as a decision on ease-of-use for the non-default path, and how accessible we want to make it.

Some feedback:

  • Can we come up with an alternative for Autonamable in AssignNameIfAutonamable? e.g. AssignNameIfGeneratedByPulumi or AssignNameIfAutoGenerated. Autonamable is ok, but I feel like we can make this clearer.

@hausdorff
Copy link
Contributor

So far we've mainly heard from users that per-resource await opt-out is very useful, but no one has asked for a global opt-out. And, it's such a big part of Pulumi that if a user asked for global opt-out, I think it would be important to really understand why it's necessary to implement before reaching for that hammer.

@joeduffy
Copy link
Member

The first time a user gave us feedback on this, he asked for a global opt-out.

@hausdorff
Copy link
Contributor

Even that person eventually decided they only wanted to opt out of Service await.

@hausdorff
Copy link
Contributor

One other thing:

  • Do we want the annotation to be skipAwait? I'm not 100% sure users will understand what this means, though I also don't have a better suggestion. FWIW, I regret calling the other property autonamed -- we won't have the opportunity to change it.

@lblackstone
Copy link
Member Author

  • Do we want the annotation to be skipAwait? I'm not 100% sure users will understand what this means, though I also don't have a better suggestion. FWIW, I regret calling the other property autonamed -- we won't have the opportunity to change it.

I can't think of a better name either. It does seem like we could relabel autonamed and include logic in the provider to support both for backwards compatibility. Probably not a huge deal though.

@joeduffy
Copy link
Member

Even that person eventually decided they only wanted to opt out of Service await.

Sure, perhaps because at the time, that was effectively the only interesting await condition we had. I'm sure they will hit another snag down the road.

I personally would have done the global opt-out and been done with it, for the "I just want kubectl-like behavior" kind of user (of whom we've encountered several). Honestly seems simpler.

If this is more of a "workaround for bugs in our await logic," I get why the per-resource approach is better. I guess I don't fully understand the motivation for the feature, but I'm gathering it's more the latter?

@lblackstone
Copy link
Member Author

I guess I don't fully understand the motivation for the feature, but I'm gathering it's more the latter?

Yes, this is to get users unblocked who hit either a bug, or a currently unsupported edge case. Most of the recent issues from the community have been in one of those categories. This allows us to unblock them without having to drop everything and work on a fix.

@lukehoban
Copy link
Member

Just to further 👍 @lblackstone, the motivation for doing this is to ensure that any latent bugs in our await logic are not major blockers for users, and that we can offer a workaround until we can fix the issue.

We did not intend this to be a new "mode" on the provider to support turning off await logic broadly - we don't really yet understand overall usability of the provider in that mode, and though it may in theory be usable, I don't think we are yet at a stage where we believe opening up such a significantly different mode of use (and testing it, documenting it, having examples use it, etc.) is worth it based on feedback.

Down the road it may be. But for now, having this as an escape hatch is the key thing we need to ensure a good user experience for the classes of feedback we've heard from many users.

@lblackstone lblackstone merged commit 3f353be into master Feb 12, 2019
@pulumi-bot pulumi-bot deleted the lblackstone/disable-await-toggle branch February 12, 2019 21:22
@hausdorff
Copy link
Contributor

Sorry, got super swamped with other stuff, but just to loop back here -- we are definitely open to changing the modality of the await conditions if that is the feedback from users. All I was saying above is that I believe this is not the feedback we're getting from users, at least, not yet. Were we to consider changing this as a global setting, I'd want to consider very carefully how this will work with Output<T>, which is likely to be a major sticking point for our model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants